Document unsafety in core::ptr #71507

CohenArthur · 2020-04-24T10:48:14Z

Contributes to #66219

I have yet to document all the unsafe blocks in the lib and would like to know if I'm headed in the right direction

Changed raw pointer name from ptr to raw_pointer to avoid confusion with the `use std::ptr` statement a few lines above. This way the crate name and pointer name are well differenciated.

rust-highfive · 2020-04-24T10:48:18Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

LeSeulArtichaut

Left you some comments :)

src/libcore/ptr/mod.rs

src/libcore/ptr/non_null.rs

src/libcore/ptr/unique.rs

LeSeulArtichaut

I left other remarks I'm not completely sure about.

I think we should also ping someone from the Libs team, cc @Mark-Simulacrum do you have time for (yet another) review?

src/libcore/ptr/mod.rs

LeSeulArtichaut · 2020-04-24T13:16:14Z

src/libcore/ptr/non_null.rs

@@ -69,6 +67,8 @@ impl<T: Sized> NonNull<T> {
    #[rustc_const_stable(feature = "const_nonnull_dangling", since = "1.32.0")]
    #[inline]
    pub const fn dangling() -> Self {
+        // SAFETY: mem::align_of() returns a valid, non-null pointer. The


I'm not sure it would be right to call the pointer returned by mem::align_of "valid". It is still dangling.

That's a good point. I thought of "valid" as in "non-null" and "usable", but it is in fact dangling and dangerous. We could simply put non-null and remove the "valid" to avoid confusion

Suggested change

// SAFETY: mem::align_of() returns a valid, non-null pointer. The

// SAFETY: mem::align_of() returns a non-null pointer. The

Thanks a lot for all your comments by the way @LeSeulArtichaut

No problem! I've got some spare time, being locked down at home :P

mem::align_of doesn't return a pointer, it returns a usize. By casting it to a pointer, you are creating the lowest non-zero *mut T that is properly aligned.

@ecstatic-morse Changed in 75a73d1

ecstatic-morse · 2020-04-24T16:32:25Z

src/libcore/ptr/non_null.rs

@@ -213,6 +221,8 @@ impl<T: ?Sized> From<Unique<T>> for NonNull<T> {
 impl<T: ?Sized> From<&mut T> for NonNull<T> {
    #[inline]
    fn from(reference: &mut T) -> Self {
+        // SAFETY: A mutable reference cannot be null, so the conditions for
+        // new_unchecked() are respected.
        unsafe { NonNull { pointer: reference as *mut T } }


You refer to new_unchecked in the SAFETY comment, but it's not what's called here. We should always be using new_unchecked to create NonNulls, even internally.

@ecstatic-morse Do you think I should add it to this PR ? I feel like changing the code is a bit outside of its scope. I'm changing the SAFETY comment

ecstatic-morse · 2020-04-24T16:43:22Z

src/libcore/ptr/unique.rs

@@ -176,6 +181,7 @@ impl<T: ?Sized> From<&mut T> for Unique<T> {
 impl<T: ?Sized> From<&T> for Unique<T> {
    #[inline]
    fn from(reference: &T) -> Self {
+        // SAFETY: A reference cannot be null


My understanding is that Unique guarantees that its referent is not aliased. This is not guaranteed by &T, so I think this From impl is unsound. Presumably we justify this because it only becomes UB when we actually create a &mut from a Unique, which is unsafe. cc @rust-lang/libs

It's important to state why all invariants are upheld when annotating these unsafe blocks, not just the one about null pointers.

@RalfJung I think mentioned recently that while Unique perhaps should provide that guarantee, it does not today.

I agree though that this impl seems very suspicious -- I believe Unique is std-internal today, so we can probably drop this impl?

In the docs for Unique::new_unchecked , it is only stated that the pointer must be non-null

I don’t believe Unique has such a language-level guarantee.

What if we just removed it? #71530

I don’t believe Unique has such a language-level guarantee.

It doesn't right now mostly because we don't know how to exactly phrase the guarantee.

I agree that an instance turning a shared reference into Unique sounds wrong, and should be removed.

The instance was added in #42959. Not sure why noone complained back then.

I think this is outside the scope of this pull request, which aims at commenting some unsafe blocks. I removed it.

src/libcore/ptr/mod.rs

src/libcore/ptr/unique.rs

rust-lang#71507 (comment) discusses whether `Unique` not actually being unique is UB. `Unique` has very limited use over `NonNull`, so it doesn’t seem worth spending a lot of effort defining an abstraction and coming up with rules about what uses of it are or aren’t sound.

steveklabnik

I cannot really review for correctness, but formatting and such looks good to me.

Mark-Simulacrum · 2020-04-28T21:20:28Z

Thanks! Can you squash this down to one commit? With that I think we'll be ready to go.

CohenArthur · 2020-04-29T08:23:21Z

@Mark-Simulacrum Thanks, I squashed it

Add documentation example to slice_from_raw_parts_mut() Add SAFETY explanations to some unsafe blocks in libcore/ptr * libcore/ptr/mod.rs * libcore/ptr/unique.rs * libcore/ptr/non_null.rs safety-mod.rs: Add SAFETY to slice_from_raw_parts(), slice_from_raw_parts_mut() slice_from_raw_parts_mut: Add documentation example safety-ptr-unique.rs: Add SAFETY to new() and cast() safety-ptr-non_null.rs: Add SAFETY to new() safety-ptr-non_null.rs: Add SAFETY to cast() safety-ptr-non_null.rs: Add SAFETY to from() impls safety-ptr-unique.rs: Add SAFETY to from() impls safety-ptr-non_null.rs: Add SAFETY to new() safety-ptr-unique.rs: Add SAFETY to new() safety-ptr-mod.rs: Fix safety explanation rust-lang#71507 (comment) safety-prt-non_null.rs: Fix SAFETY comment syntax safety-ptr-unique.rs: Fix syntax for empty() safety-ptr-non_null.rs: Fix misuse of non-null for align_of() safety-ptr-non_null.rs: Remove incorrect SAFETY comment safety-ptr-unique.rs: Remove unsound SAFETY comment safety-ptr-mod.rs: Add std comment on slice_from_raw_parts guarantee safety-ptr-unique.rs: Remove incorrect safety comment Creating a Unique from a NonNull has strict guarantees that the current implementation does not guarantee rust-lang#71507 (comment) safety-ptr: Re-adding ignore-tidy directive

Mark-Simulacrum · 2020-04-29T10:26:55Z

@bors r+ rollup

bors · 2020-04-29T10:26:57Z

📌 Commit 8558ccd has been approved by Mark-Simulacrum

@steveklabnik

…-ptr, r=Mark-Simulacrum Document unsafety in core::ptr Contributes to rust-lang#66219 I have yet to document all the `unsafe` blocks in the lib and would like to know if I'm headed in the right direction r? @steveklabnik

@ghost

Rollup of 6 pull requests Successful merges: - rust-lang#71507 (Document unsafety in core::ptr) - rust-lang#71572 (test iterator chain type length blowup) - rust-lang#71617 (Suggest `into` instead of `try_into` if possible with int types) - rust-lang#71627 (Fix wrong argument in autoderef process) - rust-lang#71678 (Add an index page for nightly rustc docs.) - rust-lang#71680 (Fix doc link to Eq trait from PartialEq trait) Failed merges: - rust-lang#71597 (Rename Unique::empty() -> Unique::dangling()) r? @ghost

doc-example: Refactor pointer name to avoid confusion

b4a11d4

Changed raw pointer name from ptr to raw_pointer to avoid confusion with the `use std::ptr` statement a few lines above. This way the crate name and pointer name are well differenciated.

rust-highfive assigned steveklabnik Apr 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2020

CohenArthur mentioned this pull request Apr 24, 2020

Document all unsafety used in libcore and libstd #66219

Closed

LeSeulArtichaut suggested changes Apr 24, 2020

View reviewed changes

LeSeulArtichaut reviewed Apr 24, 2020

View reviewed changes

Mark-Simulacrum assigned Mark-Simulacrum and unassigned steveklabnik Apr 24, 2020

ecstatic-morse reviewed Apr 24, 2020

View reviewed changes

Mark-Simulacrum reviewed Apr 24, 2020

View reviewed changes

src/libcore/ptr/mod.rs Outdated Show resolved Hide resolved

src/libcore/ptr/mod.rs Show resolved Hide resolved

src/libcore/ptr/unique.rs Outdated Show resolved Hide resolved

SimonSapin mentioned this pull request Apr 24, 2020

Remove ptr::Unique #71530

Closed

steveklabnik approved these changes Apr 26, 2020

View reviewed changes

CohenArthur force-pushed the document-unsafe-libcore-ptr branch from 33c4981 to 8558ccd Compare April 29, 2020 08:22

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2020

This was referenced Apr 29, 2020

Rollup of 4 pull requests #71673

Closed

Rollup of 6 pull requests #71689

Merged

bors merged commit d9761da into rust-lang:master Apr 29, 2020

workingjubilee mentioned this pull request Sep 8, 2020

Remove a stray ignore-tidy-undocumented-unsafe #76493

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document unsafety in core::ptr #71507

Document unsafety in core::ptr #71507

CohenArthur commented Apr 24, 2020

rust-highfive commented Apr 24, 2020

LeSeulArtichaut left a comment

LeSeulArtichaut left a comment

LeSeulArtichaut Apr 24, 2020

CohenArthur Apr 24, 2020 •

edited

Loading

CohenArthur Apr 24, 2020

LeSeulArtichaut Apr 24, 2020

ecstatic-morse Apr 24, 2020

CohenArthur Apr 26, 2020

ecstatic-morse Apr 24, 2020

CohenArthur Apr 26, 2020

ecstatic-morse Apr 24, 2020 •

edited

Loading

Mark-Simulacrum Apr 24, 2020

LeSeulArtichaut Apr 24, 2020

SimonSapin Apr 24, 2020

RalfJung Apr 25, 2020

RalfJung Apr 25, 2020

CohenArthur Apr 26, 2020 •

edited

Loading

steveklabnik left a comment

Mark-Simulacrum commented Apr 28, 2020

CohenArthur commented Apr 29, 2020

Mark-Simulacrum commented Apr 29, 2020

bors commented Apr 29, 2020

	// SAFETY: mem::align_of() returns a valid, non-null pointer. The
	// SAFETY: mem::align_of() returns a non-null pointer. The

Document unsafety in core::ptr #71507

Document unsafety in core::ptr #71507

Conversation

CohenArthur commented Apr 24, 2020

rust-highfive commented Apr 24, 2020

LeSeulArtichaut left a comment

Choose a reason for hiding this comment

LeSeulArtichaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CohenArthur Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CohenArthur Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

steveklabnik left a comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Apr 28, 2020

CohenArthur commented Apr 29, 2020

Mark-Simulacrum commented Apr 29, 2020

bors commented Apr 29, 2020

CohenArthur Apr 24, 2020 •

edited

Loading

ecstatic-morse Apr 24, 2020 •

edited

Loading

CohenArthur Apr 26, 2020 •

edited

Loading